Conversation
Signed-off-by: Dmytro Rashko <[email protected]>
EItanya
reviewed
Jun 30, 2026
EItanya
reviewed
Jun 30, 2026
Bump dependencies to clear all 8 govulncheck source-mode findings: - golang.org/x/net 0.50.0 -> 0.55.0 (GO-2026-5026, GO-2026-4918, GO-2026-4559) - github.com/cilium/cilium 1.19.0 -> 1.19.3 (GO-2026-5400, GO-2026-4856) - go.opentelemetry.io/otel 1.40.0 -> 1.43.0, otlploghttp 0.16.0 -> 0.19.0 (GO-2026-4985) - github.com/go-jose/go-jose/v4 4.1.3 -> 4.1.4 (GO-2026-4945) - github.com/anchore/syft 1.32.0 -> 1.42.3 (GO-2026-4809) The only remaining govulncheck reports are 5 uncalled github.com/docker/docker advisories that have no fixed version available upstream. Also carry the in-flight go-sdk migration work: relax the inferred input schema (drop the auto-Required list and allow additional properties) so tools keep the pre-migration calling contract, add the toolResultText e2e helper for readable failures, and document the typed MCP tool I/O conventions. Signed-off-by: Dmytro Rashko <[email protected]>
dimetron
force-pushed
the
feature/mcp-sdk-migration
branch
from
June 30, 2026 17:07
cbf73e2 to
303449c
Compare
dimetron
marked this pull request as draft
June 30, 2026 18:29
Add a typed mcp_inspect tool that echoes its input and returns all HTTP headers received with the MCP request, for debugging client requests. Headers are canonicalized and sorted for stable output. Also bump github.com/modelcontextprotocol/go-sdk v1.6.1 -> v1.7.0 and the go directive to 1.27.0 in support of the migration work. Signed-off-by: Dmytro Rashko <[email protected]>
The pinned v1.63.4 predates the go 1.27 directive and cannot load the module. Add a version: "2" config and pin the v2 module path so `make lint` (part of `make test`) runs on the migrated tree. Signed-off-by: Dmytro Rashko <[email protected]>
Step 3 of the go-sdk migration: replace the dynamic
map[string]interface{} context with a concrete map[string]string so no
untyped map remains in the error path.
Callers that passed non-string values are converted at the call site:
- prometheus: status_code (int) -> decimal string
- helm: helm_args ([]string) -> space-joined string
Signed-off-by: Dmytro Rashko <[email protected]>
Step 12 of the go-sdk migration: the read-only scan/report tools built
their JSON responses from map[string]interface{} literals. Introduce
concrete output structs for all seven handlers and decode them typed in
the tests, so no untyped map remains in the kubescape response path.
Conditional fields (seccomp_profile, fix_state/fix_versions) keep their
omit-when-absent behaviour via omitempty and a pointer.
Signed-off-by: Dmytro Rashko <[email protected]>
Step 15 cleanup: gofmt the ToolError field alignment, and decode the logger trace_id assertion into an anonymous struct. The mcp_test payload stays a map because jsonschema.Resolved.Validate rejects structs for object schemas (google/jsonschema-go#23) — noted inline. Signed-off-by: Dmytro Rashko <[email protected]>
Complete the go-sdk migration by removing the last untyped tool I/O. Every
handler previously returned Out=any, so the SDK could not infer an output
schema, populate StructuredContent, or validate the result. All 145 handlers
now return a concrete Out type.
Raw CLI text goes through the shared mcp.TextOutput wrapper ({output: "..."})
via new mcp.TextResult / mcp.TextError / mcp.TextOf helpers. Structured
responses return their own DTO. Text stays in Content as before, so existing
clients are unaffected; StructuredContent is now populated too.
Three SDK behaviours drove the design, each confirmed against a live
in-memory server:
- json.RawMessage is inferred as a byte slice, not arbitrary JSON, and
validation then rejects real objects/arrays. pkg/prometheus now re-indents
dynamic API JSON with json.Indent instead of decoding into interface{}.
- Zero values are validated on error paths too, so a map field without
omitempty makes every error return fail with "validating tool output".
pkg/kubescape gained omitempty on its map fields; CheckStatus.Details is
typed as []PodCheckEntry instead of interface{}. Slices infer as nullable
and need no change.
- Types with custom JSON marshallers can fail schema inference, which panics
AddTool at registration. v1beta1.WorkloadConfigurationScan is one such type,
so handleGetConfigurationScan returns mcp.TextOutput. The new
cmd/tools_output_schema_test.go registers every provider and fails if any
Out type cannot produce a valid schema.
test/e2e/helpers_test.go now returns *mcp.CallToolResult and []*mcp.Tool
instead of interface{}, and the input-schema regression test exercises a real
client->server call with typed partial arguments rather than validating a
map[string]any fixture by hand.
The only remaining "any" tokens are generic type parameters ([T any]), which
bind concrete types at every call site.
AGENTS.md documents the typed-output contract, the TextResult/TextError/
TextOf helpers, and the three output-schema pitfalls above.
Verified: gofmt clean, go build ./... and go vet ./... pass, make lint reports
0 issues, go test ./pkg/... ./internal/... ./cmd/... passes 19/19 packages,
every pkg/ package is above the 80% coverage gate, and an in-repo grep finds
no interface{}, no Out=any, and no untyped maps.
Signed-off-by: Dmytro Rashko <[email protected]>
Both Go jobs pinned `go-version: '^1.26.1'`, which resolves to 1.26.8, but
go.mod requires go >= 1.27.0. actions/setup-go@v6 also sets GOTOOLCHAIN=local
(an intentional change in setup-go 1d76b95), so the toolchain is never
auto-downloaded and the job dies during `make test` -> `make build`:
go: go.mod requires go >= 1.27.0 (running go 1.26.8; GOTOOLCHAIN=local)
This failed before a single test ran, in both go-unit-tests and go-e2e-tests.
The `build` job was unaffected because the Dockerfile uses
chainguard/go:latest, which already ships 1.27.
Use `go-version-file: 'go.mod'` so CI tracks the module's own Go directive and
cannot drift when the directive is bumped again. The Docker build and the lint
config (pinned for the go 1.27 directive) were already consistent with 1.27;
only the two setup-go pins were stale.
Also correct the Go version listed in the AGENTS.md repository tree (1.25.6 ->
1.27.0), which is the same drift that caused this.
Verified under the exact CI condition (GOTOOLCHAIN=local, go.mod at 1.27.0):
go build ./..., go vet ./... and go test ./pkg/... ./internal/... ./cmd/...
all pass, and make lint reports 0 issues. Reproduced the original failure as a
control by running go 1.26.8 with GOTOOLCHAIN=local against the 1.27.0
directive.
Signed-off-by: Dmytro Rashko <[email protected]>
dimetron
marked this pull request as ready for review
September 21, 2026 06:45
The e2e suite reported success without exercising the migrated behaviour: three specs skipped silently, a helper swallowed tool errors, and setup raced kube-proxy. Fix the harness and assert the typed-output contract end to end. Assertions now decode structuredContent into the shared mcp.TextOutput DTO via decodeTextOutput, so a handler regressing to Out=any fails the suite. Applied to the k8s, helm, istio, cilium and argo specs; a new istio_version spec runs against the installed control plane. Bugs fixed: - ciliumStatus was the only MCP helper that did not check result.IsError, so its spec passed even when the tool returned an error. It now returns an error. - The helm spec never ran: helmListReleases sent all_namespaces as the string "true" while the tool declares a boolean, so every call failed input validation and the spec skipped itself. It now sends a bool. - InstallKAgentTools waited on pod status.phase only. phase=Running precedes the readiness probe (initialDelaySeconds=15), so GetMCPClient could connect to a server that had not begun serving. It now waits for the Ready condition and probes the NodePort before the suite starts. - CreateNamespace/DeleteNamespace ignored in-flight namespace deletion, so consecutive runs failed with "unable to create new content ... because it is being terminated". Both now wait for the namespace to settle. Note --ignore-not-found exits 0 for a missing namespace, so the waits key on empty output rather than on an error. - GetMCPClient retries the initialize handshake, because the NodePort resets connections until kube-proxy programs the new endpoint. - The helm install context (120s) was shorter than helm's own timeout, killing helm with "signal: killed"; the context now outlives it, and --timeout is 3m to clear the readiness delay. Adds an opt-in Cilium lifecycle spec (E2E_CILIUM_LIFECYCLE=true) that installs Cilium through cilium_install_cilium, verifies status, then uninstalls through cilium_uninstall_cilium. It is opt-in because it replaces the cluster CNI, which Kind does not use by default and CI cannot tolerate. Installing a CNI resets pod networking on the node and drops the pod's own long-lived MCP session, so the spec reconnects afterwards. The uninstall runs inside the It rather than DeferCleanup: the ordered container's AfterAll deletes the namespace first, leaving no server to call. A host-side DeferCleanup removes a leaked DaemonSet so a failure cannot poison later runs. Also set tools.metrics.port=8085 in test-values-e2e.yaml. Without it the deployment template emits containerPort 8084 twice and Helm 4 rejects the manifest with "duplicate entries for key [containerPort=8084]", which broke every deploy. Verified locally on a Kind cluster with the repo's NodePort mappings: - default suite: 24 passed, 0 failed, 2 skipped (the two Cilium specs) - with E2E_CILIUM_LIFECYCLE=true: 25 passed, 0 failed, 1 skipped, Cilium installed and then uninstalled, nothing leaked - 3 consecutive default runs passed, where the same sequence previously failed roughly half the time - gofmt and go vet clean; unit tests unaffected (19/19 packages pass) Signed-off-by: Dmytro Rashko <[email protected]>
The suite previously touched about 8 of the 126 advertised tools. Add a
data-driven sweep that drives all 80 tools the server registers in read-only
mode and asserts two invariants for each:
- the call completes without a protocol/transport error, and
- the result honours the typed-output contract: a success carries
structuredContent (so a handler regressing to Out=any fails the suite), and
a failure reports IsError with a readable message rather than breaking
transport.
The read-only set is taken from the providers themselves: the sweep registers
argo, cilium, helm, istio, k8s, kubescape, prometheus and utils with
readOnly=true and uses the resulting tool list as the safety rail. No name
pattern guessing, and a write-capable tool cannot be reached from the sweep's
invocation list. This matters because an earlier attempt at a regex
classification wrongly flagged helm_repo_update, which the server registers
read-only (it only refreshes the local chart cache).
Tools needing identifiers that exist only alongside a live dependency (Cilium
endpoint/service/recorder IDs, an existing Helm release, a vulnerability
manifest name) are enumerated with an explicit skip reason rather than called
with invented arguments, which would only re-test input validation. Tools whose
backing dependency is absent on a Kind cluster (Cilium on kindnet, no Prometheus
server, no Kubescape operator) legitimately answer with a tool error; the sweep
records those instead of failing, so the run reports
ok=24 toolerr=32 skipped=24 for 80 covered tools.
The sweep runs inside the ordered k8s container, before AfterAll deletes the
namespace, so the deployed server is still reachable.
Also fixes a bug introduced earlier with the namespace race fix: CreateNamespace
waited for the namespace to be absent, but a healthy namespace left over from a
previous run must be reused, not waited out. It now waits for the namespace to be
usable (gone, or present and Active). DeleteNamespace no longer blocks, since
deletion may linger on CRD finalizers and would turn a slow finalizer into an
AfterAll failure; CreateNamespace is the only place the wait matters.
Verified on a local Kind cluster with the repo's NodePort mappings: the suite
passes 26 specs (0 failed) and twice consecutively; gofmt and go vet are clean
and unit tests still pass 19/19 packages.
Signed-off-by: Dmytro Rashko <[email protected]>
Go dependencies, direct:
- github.com/modelcontextprotocol/go-sdk v1.7.0 -> v1.8.0 (the release that
prompted this). It adds no new protocol revision; the work is transport
hardening plus ServerOptions.SupportedProtocolVersions and SetCacheable. The
output-schema machinery the typed-output migration depends on (toolForErr,
setSchema, ToolHandlerFor, AddTool) is byte-for-byte unchanged, so no handler
changes were needed.
- k8s.io/{api,apimachinery,client-go,apiextensions-apiserver} v0.35.3 -> v0.37.0
- go.opentelemetry.io/otel* v1.43.0 -> v1.46.0
- github.com/prometheus/client_golang v1.23.2 -> v1.24.1, client_model v0.6.2 -> v0.6.3
- github.com/kubescape/k8s-interface v0.0.203 -> v0.0.221
- github.com/onsi/ginkgo/v2 v2.27.2 -> v2.33.0, gomega v1.38.2 -> v1.43.1
- github.com/stretchr/testify v1.11.1 -> v1.12.1
- go directive 1.27.0 -> 1.27.1
github.com/kubescape/storage v0.0.239 -> v0.0.300, not the latest v0.2.0. That
module is imported directly for its v1beta1 API types and generated clientset,
and upstream removed both the types and the client methods this code uses
(v1beta1.ExecCalls, OpenCalls, HTTPEndpoint, NetworkConnections,
CommunicationType, NetworkPort, SingleSeccompProfile, and the
ApplicationProfiles/WorkloadConfigurationScans/NetworkNeighborhoods client
methods). v0.0.300 is the newest version that still exposes them; going further
requires rewriting the kubescape DTOs, which is out of scope here.
Bundled CLI versions, checked against each project's latest release:
- istioctl 1.30.1 -> 1.31.0
- argo 1.9.0 -> 1.10.0
- kubectl 1.36.2 -> 1.37.0
- helm 4.2.2 -> 4.3.0
- cilium 0.19.4 -> 0.20.0
make check-releases now reports all six checks green.
Verified: go build ./... and go vet ./... pass; go test ./pkg/... ./internal/...
./cmd/... passes 19/19 packages; make lint reports 0 issues; gofmt clean. The
image was rebuilt with make docker-build and each binary was executed inside it
to confirm the versions actually installed, rather than trusting the pin:
kubectl v1.37.0, helm v4.3.0, istioctl 1.31.0, cilium-cli v0.20.0,
kubectl-argo-rollouts v1.10.0, and the server reporting go1.27.1.
Signed-off-by: Dmytro Rashko <[email protected]>
Add the specification documents produced for the mark3labs/mcp-go -> official go-sdk migration, plus a Claude Code entry point. specs/ holds the migration materials: requirements, design, plan, research comparison and the skill notes. specs/tools/001-migrate-mcp-go-to-official-sdk/ is the current copy and carries the run summary; the older specs/migrate-mcp-go-to-official-sdk/ copy shares five byte-identical files. CLAUDE.md mirrors the repository guide for Claude Code. Signed-off-by: Dmytro Rashko <[email protected]>
CLAUDE.md documented the pre-migration mark3labs API throughout: the tool registration example used mcp.NewTool with WithString/WithDescription, handlers used request.RequireString()/RequireBool(), and results used mcp.NewToolResultText(). None of that exists in the codebase after the go-sdk migration, so the examples would not compile and an agent following them would write handlers that break. It also had no mention of the typed-output contract that the migration introduced. It referenced things that are not in the repository either: pkg/logger (logging lives in internal/logger), make coverage-report (no such target), coverage.md, and test/integration/. Replace the duplicated and outdated content with a short file that directs readers to AGENTS.md as the single source of truth and adds only what AGENTS.md does not cover: the run-local flags, the registration/typed-output points that are easy to get wrong, and the logging package location. Also correct a claim both files made: that CI enforces the 80% coverage threshold. The go-unit-tests job runs "go test -v -cover", which reports coverage but has no gate, so a threshold breach cannot fail the build. Both files now describe 80% as the standard to check manually, and note that internal/commands and internal/cmd sit below it. Every claim in the rewritten file was checked against the repository: the CLI flags against "--help", each linked file for existence, per-package coverage with "go test -cover", the DCO check against the open pull request, and the claim that no provider imports the SDK directly with grep (all 127 registrations go through internal/mcp). Signed-off-by: Dmytro Rashko <[email protected]>
… findings Findings from a codex review of this branch. 1. mcp_inspect disclosed credentials (high). The HTTP transport hands handlers the raw inbound header set, so req.Header contains Authorization, Cookie and anything else the client sent. The tool returned every value to any caller able to invoke it, exposing the caller's own bearer token and session secrets. Header values are now withheld when the canonical name contains a credential-bearing fragment (authorization, cookie, token, secret, api-key, credential, password, authenticat, session, bearer, jwt, signature). An earlier exact-match list was insufficient and a second review pass caught it: it missed Private-Token, X-Amz-Security-Token, X-Access-Token and similar real-world names, so the policy is deny-by-substring. Over-redacting a debugging value costs nothing; leaking a credential does not. Header names are still returned and the received value count is preserved, so the tool remains useful for debugging which headers arrived. 2. Tool-level failures left the span status unset (medium). A handler signalling failure via IsError=true with a nil Go error incremented the Prometheus failure counter but left the OTel span status unset, because both RecordError and SetStatus sat inside `if err != nil`. Traces therefore disagreed with metrics and the span was neither Ok nor Error. The middleware now marks such spans Error using the tool's message, and sets is_error on the span. 3. kubescape_get_vulnerability_details emitted its payload twice (medium). For a non-object (array) Out the SDK appends the serialised value as an extra TextContent block, so returning the matches as Out *and* marshalling them into Content produced the vulnerability list twice for clients reading Content. The handler now returns `nil, matches, nil` and lets the SDK serialise. Tests cover each fix and were checked for vacuity: reverting the span fix makes TestToolMiddleware_MarksSpanForToolLevelError fail with "span status: expected Error, got Unset", and TestIsSensitiveHeader pins both directions of the redaction policy (18 credential names withheld, 8 innocuous headers kept visible). Verified end to end by deploying the image and sending Authorization, Private-Token, X-Amz-Security-Token and X-Access-Token: all returned [REDACTED] while X-Request-Id stayed visible. Verified: go build ./..., go vet ./... and gofmt clean; go test ./pkg/... ./internal/... ./cmd/... passes 19/19 packages; make lint reports 0 issues; the e2e suite passes 26 specs with the read-only sweep still covering 80/80 tools. Signed-off-by: Dmytro Rashko <[email protected]>
Both branches bumped dependencies, so the resolution needed care rather than a blanket "take ours": main's most recent commit is itself a CVE bump, and taking our side wholesale would have silently reverted two of its fixes. go.mod / go.sum - Kept main's google.golang.org/grpc v1.83.2 (ours was v1.83.1) and github.com/buger/jsonparser v1.1.2, both part of main's CVE bump. A naive merge kept grpc at the older version. - Kept this branch's higher versions everywhere they were newer (go-sdk v1.8.0, k8s.io v0.37.0, otel v1.46.0 line, kubescape/storage v0.0.300 and ~40 others). - Re-ran `go mod tidy`, which recomputed the graph. Nine modules present on main but absent here disappear with it (mark3labs/mcp-go and its tree: jsonparser, goccy/go-yaml, invopop/jsonschema, mailru/easyjson, go-ordered-map, generic-list-go, go-snaps, yaml.v3). That is the point of the migration - the old SDK is no longer a dependency - not a lost CVE pin. Makefile - Kept this branch's CLI pins: istioctl 1.31.0, argo 1.10.0, kubectl 1.37.0, helm 4.3.0, cilium 0.20.0. Main had bumped only argo (to the same 1.10.0); ours is a strict superset. test/e2e/k8s_test.go - Kept both imports (os and time). - Main independently added an Eventually retry around GetMCPClient for the same NodePort-readiness race this branch fixed inside the helper. Both are harmless together, so main's wrapper is retained. Verified on the merged tree: go build ./... and go vet ./... clean, gofmt clean, go test ./pkg/... ./internal/... ./cmd/... passes 19/19 packages, golangci-lint reports 0 issues, and the helm chart still renders distinct containerPorts 8084/8085 (the duplicate-port fix survives main's new image helpers). Signed-off-by: Dmytro Rashko <[email protected]>
Removes material that should not ship in the pull request. specs/ holds the working documents from the SDK migration (requirements, design, plan, research comparison, skill notes). Nothing in the repository depends on them - no CI job, Makefile target, or doc link references the path - so they were pure review noise in the diff. cmd/testdata/tool_names_v0.2.1.txt and cmd/tools_regression_test.go are removed as a pair. They arrived in the same commit (ef66d5a) and are hard-coupled: the test opens the golden file with require.NoError, so deleting the data alone fails the build with "open testdata/tool_names_v0.2.1.txt: no such file or directory". Removing the file alone was therefore not an option. The dropped test asserted that tool names shipped in v0.2.1 are still registered, which is a real check, but the golden baseline is 132 lines of v0.2.1-era data that the reviewer flagged as not belonging in this PR. Tool registration is still covered by TestEveryToolHasValidOutputSchema (every advertised tool must infer a valid output schema) and by the e2e sweep, which asserts the server exposes all 80 read-only tools. The e2e comment that referenced the removed test name is updated accordingly. Verified: go build ./..., go vet ./... and gofmt clean; go test ./pkg/... ./internal/... ./cmd/... passes 19/19 packages; golangci-lint reports 0 issues; the e2e suite still compiles under -tags=test. 3727 lines removed. Signed-off-by: Dmytro Rashko <[email protected]>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.